Interface SCIP methods - #1237
Conversation
There was a problem hiding this comment.
@Joao-Dionisio were there some changes in the ruff config? I didn't even touch these lines in my commits.
If it's ok I can refactor it to use the more modern X | Y syntax as suggested in the error message
There was a problem hiding this comment.
This is likely just a new ruff version flagging some errors. I'd just merge without caring much about this, but if you're offering to fix this, I'm more than happy to accept :)
I'll take a little bit to review, but everything seems to be in order. Can you please make sure that the optimized_model() manages to find primal solutions?
There was a problem hiding this comment.
I checked that optmized_model() wasn't able to find any primal solution, so I 've increased the node limit parameter and now it finds 1.
Regarding the X | Y syntax, I only changed it in scip.pyi to pass the pipeline checks and because this syntax is already used there.
I was thinking about replacing typing.Union in the entire repo, but in general such fixes are considered unsafe for Python versions prior to 3.10 (see for reference: https://docs.astral.sh/ruff/rules/non-pep604-annotation-union/) and pyproject.toml only requires python>=3.8. Maybe there's no point in touching it until the project moves to python>=3.10?
There was a problem hiding this comment.
Yes, we tend to prefer supporting as many versions as possible, and 3.10 is not thaaat old yet.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1237 +/- ##
==========================================
- Coverage 57.91% 57.29% -0.62%
==========================================
Files 26 27 +1
Lines 5807 5936 +129
==========================================
+ Hits 3363 3401 +38
- Misses 2444 2535 +91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Sorry for the delay @adamj34 ! I'm not as free in September as I was expecting to be, but I'll still manage to take a closer look. I'm not sure what the code coverage bot is complaining about (am AFK), but it'd be nice to ensure that everything is tested. |
Co-authored-by: João Dionísio <57299939+Joao-Dionisio@users.noreply.github.com>
| # very simple model with 2 primal solutions | ||
| # created because getting more than 1 primal solution from random_mip_1 requres setting a large node limit, which slows down the tests | ||
| @pytest.fixture | ||
| def optimized_model_with_primal_solutions(): |
There was a problem hiding this comment.
There's no guarantee that this will remain working for future SCIP versions, but I suppose the same would be true for every model.
There was a problem hiding this comment.
While I'm not sure how likely it was to break in the future, I think it would be better to have something more reliable than assuming that an x number of node limit will produce y number of solutions.
I came up with choosing a simple model and providing solutions by hand, so that it's easy to reason about all possible solutions. I also added a few comments in the tests to make them clearer.
Please let me know if this approach is better.
| leaves, children, siblings = optimized_model.getOpenNodes() | ||
| open_nodes = leaves + children + siblings | ||
| manual_avg_lowerbound = 0.0 | ||
| if len(open_nodes) > 0: |
There was a problem hiding this comment.
This is 0 if the model is solved to optimality, so I suppose we should assert that it isn't.
There was a problem hiding this comment.
I deleted the if statement.
There was a problem hiding this comment.
If the model is solved to optimality, getFocusNode() will return None and the test will raise an error. I think a pre-condition that the model is not solved to optimality would be better
|
|
||
|
|
||
| def test_getAvgDualbound(optimized_model): | ||
| avg_dualbound = optimized_model.getAvgDualbound() |
There was a problem hiding this comment.
This has the same original vs transformed space fragility. But, I suppose it's not a big deal, it's a minimization problem, and any SCIP changes to the way it handles this will error out very loudly everywhere.
There was a problem hiding this comment.
I think that for a minimization problem getAvgDualbound() and getAvgLowerbound() will be exactly the same.
For a maximization problem, one will be the negative of the other. That's why I included or in the assert statement.
There was a problem hiding this comment.
We could test it similarly to test_getAvgLowerbound but there's no getDualbound() method defined on Node. This is the reason behind using a comparison to getAvgLowerbound for testing function.
There was a problem hiding this comment.
I think that for a minimization problem getAvgDualbound() and getAvgLowerbound() will be exactly the same.
The sign will be the same, but SCIP may introduce an objective offset and objective scale, for example. But, without presolving, I suppose that's not the case.
|
Thank you for your suggestions and comments @Joao-Dionisio! I believe I've addressed all of them. If there's anything that requires further improvement, then let me know. Regarding the Codecov complaints, I double checked if all methods that I added were tested. It seems that PS I hope that these reviews aren't taking too much time away from your PhD 😊 |
|
Thank you! We're really in the final stages, less than a month to go, thankfully 🙏 |
| @@ -1,16 +1,49 @@ | |||
| from pyscipopt.scip import Model | |||
There was a problem hiding this comment.
Why not import from pyscipopt directly?
Adds the following methods:
getNNodesLeft(),getNRuns(),getNReoptRuns(),addNNodes(),getDeterministicTime(),getAvgDualbound(),getMaxTotalDepth(),getNBacktracks(),getFocusNode(),getAvgLowerbound(),getFirstPrimalBound(),getLowerboundRoot(),getUpperbound(),getNObjlimLeaves()Adds tests in
tests/test_statistics.pyandtests/test_node.pyfor migrated methodsAdds function signatures to
scip.pyifor type hintsUpdates
CHANGELOG.md